Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5473a234
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| **kwargs | ||
| ) | ||
|
|
||
| def js_proto_library(name, proto, copy_types = None): |
There was a problem hiding this comment.
Can't the proto target have many proto files? Doesn't that mean we need to output many "types" files?
There was a problem hiding this comment.
yes, but I'm also hoping to avoid making the user repeat the list of output files like we did in rules_ts... working on it
no, it's the same pattern as the other |
| gen_file = "_{}.gen_{}.d.ts".format(name, i) | ||
| diff_target = "{}.diff_{}".format(name, i) | ||
| select_file(name = gen_file, srcs = "_{}.types".format(name), subpath = src_file) | ||
| diff( |
There was a problem hiding this comment.
@kormide did you guys think about any APIs to support multiple files?
There was a problem hiding this comment.
Not yet. Open to suggestions. @alexeagle and I are also discussing async.
|
|
||
| def _js_proto_library_impl(ctx): | ||
| return [ | ||
| OutputGroupInfo(types = ctx.attr.proto[JsInfo].types), |
There was a problem hiding this comment.
Is it ok this has no DefaultInfo(files) with the JsInfo.sources like js_library does?
I think having no DefaultInfo(runfiles) is probably correct though? Or that might be a discussion+test for another day...
|
You can rebase/pull to get my workaround for the bazel 9.0.1 issue (pinning to 9.0.0 for now). |
| def js_proto_library(name, proto, copy_types = []): | ||
| """Wrap a proto_library to invoke the js_proto_toolchain. | ||
|
|
||
| Can copy .d.ts type definitions back to the source folder. |
There was a problem hiding this comment.
Will users want to create a js_proto_library() for every proto_library(), so that all .d.ts files get copied back to the source folder? This makes me wonder if js_proto_library() should use an aspect for this, so that you can get all the .d.ts files copied over without needing to replicate the whole proto_library dependency graph.
There was a problem hiding this comment.
Users probably do want a lang_proto_library for each proto_library, that's already the common practice and it's Gazelle's job to create those targets in the BUILD file.
If we only wanted to have an aspect on a js_library -> proto_library edge, we already have one of those. I could experiment with the DX of doing it without any js_proto_library targets in the BUILD file.
I'll get an example landed first so we can base our PRs from that.
Also fix a bug where `js_library` deps ran the proto aspect, but js_binary/js_test did not. /cc @acozzette
alexeagle
force-pushed
the
diff.bzl.d.ts
branch
from
8439a39 to
8763c48
Compare
|
Alex Eagle seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
5 participants
This is a missing feature compared with the now-deprecated ts_proto_library
Thanks to @kormide for getting the new diff.bzl out the door!